feat(NODE-6403): Add CSOT support to client bulk write #4261

baileympearson · 2024-10-01T19:20:48Z

Description

What is changing?

This PR adds support for CSOT to the client bulk write API.

Is there new documentation needed for these changes?

What is the motivation for this change?

Double check the following

Ran npm run check:lint script
Self-review completed using the steps outlined here
PR title follows the correct format: type(NODE-xxxx)[!]: description
- Example: feat(NODE-1234)!: rewriting everything in coffeescript
Changes are covered by tests
New TODOs have a related JIRA ticket

baileympearson · 2024-10-08T14:30:59Z

src/cmap/wire_protocol/on_data.ts

@@ -116,6 +116,7 @@ export function onData(
    emitter.off('data', eventHandler);
    emitter.off('error', errorHandler);
    finished = true;
+    timeoutForSocketRead?.clear();


This was the cause of the hanging test suites - some CSOT tests spawn huge timeouts that we never clean up.

baileympearson · 2024-10-08T14:33:32Z

test/integration/client-side-operations-timeout/node_csot.test.ts

-              yield { parse: () => writeErrorsReply };
-            } else {
-              yield (await realIterator.next()).value;
+            try {


With the change on onData, we need to be sure we call finally in the mock too.

baileympearson · 2024-10-08T14:34:15Z

test/integration/collection-management/collection_db_management.test.ts

@@ -16,7 +16,7 @@ describe('Collection Management and Db Management', function () {
  });

  it('returns a collection object after calling createCollection', async function () {
-    const collection = await db.createCollection('collection');
+    const collection = await db.createCollection(new ObjectId().toHexString());


I ran the test suite twice and this failed the second time - I can remove this if we want but this guarantees we never create the same collection twice.

These tests should be more responsible about their side effects, like: returns true after calling dropDatabase is dropping the common default 'test' db. This is fine to leave in, but we could do a bit more improvement in the before/after dept. up to u!

baileympearson · 2024-10-08T20:02:26Z

test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts


        client = this.configuration.newClient({ timeoutMS: 2000, monitorCommands: true });
+        client.on('commandStarted', filterForCommands('bulkWrite', writes));
      });



This prose test incorporates changes from mongodb/specifications#1672.

Clarify drivers may provide maxWriteBatchSize

where are we providing that value?

ah in the makeMultiBatchWrite helper, got it

nbbeeken · 2024-10-10T14:36:40Z

src/timeout.ts

@@ -171,7 +171,7 @@ function isCSOTTimeoutContextOptions(v: unknown): v is CSOTTimeoutContextOptions

 /** @internal */
 export abstract class TimeoutContext {
-  static create(options: TimeoutContextOptions): TimeoutContext {
+  static create(options: Partial<TimeoutContextOptions>): TimeoutContext {


Previously we always needed to make sure there was a serverSelectionTimeoutMS, shouldn't that continue to be required?

No, I believe that was an oversight. Why was it required in the first place?

Isn't it always necessary to provide because serverSelection is the max bound of that step? @W-A-James may be more familiar than I

Eventually I think we'll want to create timeout contexts that apply to subparts of the driver. For example, authentication and monitoring execute commands directly on the connection. I envisioned us creating timeout with TimeoutContext.create() and passing the result in directly.

These places do not have a server selection timeout provided and do not need one.

Since this isn't how we use it now can we keep making sure we pass in this value to not make any mistakes for the current use cases? making it Partial later when there is no need for it shouldn't be too much effort, but we're still WIP on the CSOT use case that does need it.

src/utils.ts

src/operations/client_bulk_write/executor.ts

nbbeeken · 2024-10-10T14:44:14Z

test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts


        client = this.configuration.newClient({ timeoutMS: 2000, monitorCommands: true });
+        client.on('commandStarted', filterForCommands('bulkWrite', writes));
      });



Clarify drivers may provide maxWriteBatchSize

where are we providing that value?

test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts

nbbeeken · 2024-10-10T14:55:19Z

test/integration/crud/client_bulk_write.test.ts

+        });
+      });
+
+      it('timeoutMS is used as the timeout for the bulk write', metadata, async function () {


it "throws MongoOperationTimeoutError when the server does not reply in time"

Not necessarily exactly what I've written but the expected API is thrown error vs "using X as the timeout" is the implementation right?

No, the expectation is that the operation takes no longer than timeoutMS. That we throw a timeout error is just a manifestation of this in our public API.

That manifestation (throwing) is the expected behavior right? if the expectation is that it takes no longer than timeoutMS then shouldn't there be assertions that measure the distance between a start and end time?

No. The expected behavior is that the operation times out within timeoutMS. The timeout error is how we communicate this to users but if that's all we consider the "semver API" - what is stopping us from immediately throwing a timeout error before executing any commands?

I've adjusted the tests to measure the execution time of each operation and assert it is within a range of timeoutMS.

The timeout error is how we communicate this to users

Isn't MongoOperationTimeoutError still the expected error? I thought these tests checked that but now it is MongoError.

what is stopping us from immediately throwing a timeout error before executing any commands?

Yea I agree, I think this morning we realized we don't really have coverage for meeting the deadline in general because the spec tests only expect error, but we should think about introducing time-taken assertions.

I changed them to MongoError because while timeout errors are the actual error thrown, that's not what this test is asserting.

Should we also have tests that separately cover client.bulkWrite throws the expected timeout error? Are there CSOT spec tests for client.bulkWrite? that would suffice

Any other error thrown within the expected deadline isn't capturing the expected behavior, how we communicate the timeout matters if one is to write code against that behavior.

test/integration/crud/client_bulk_write.test.ts

nbbeeken · 2024-10-10T15:29:11Z

test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts


        client = this.configuration.newClient({ timeoutMS: 2000, monitorCommands: true });
+        client.on('commandStarted', filterForCommands('bulkWrite', writes));
      });



ah in the makeMultiBatchWrite helper, got it

test/integration/crud/client_bulk_write.test.ts

nbbeeken · 2024-10-10T16:02:03Z

test/integration/crud/client_bulk_write.test.ts

+        });
+      });
+
+      it('timeoutMS is used as the timeout for the bulk write', metadata, async function () {


That manifestation (throwing) is the expected behavior right? if the expectation is that it takes no longer than timeoutMS then shouldn't there be assertions that measure the distance between a start and end time?

test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts

src/utils.ts

nbbeeken · 2024-10-10T18:27:42Z

test/integration/crud/client_bulk_write.test.ts

+        });
+      });
+
+      it('timeoutMS is used as the timeout for the bulk write', metadata, async function () {


The timeout error is how we communicate this to users

Isn't MongoOperationTimeoutError still the expected error? I thought these tests checked that but now it is MongoError.

what is stopping us from immediately throwing a timeout error before executing any commands?

Yea I agree, I think this morning we realized we don't really have coverage for meeting the deadline in general because the spec tests only expect error, but we should think about introducing time-taken assertions.

test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts

nbbeeken · 2024-10-10T18:40:49Z

src/timeout.ts

@@ -171,7 +171,7 @@ function isCSOTTimeoutContextOptions(v: unknown): v is CSOTTimeoutContextOptions

 /** @internal */
 export abstract class TimeoutContext {
-  static create(options: TimeoutContextOptions): TimeoutContext {
+  static create(options: Partial<TimeoutContextOptions>): TimeoutContext {


Since this isn't how we use it now can we keep making sure we pass in this value to not make any mistakes for the current use cases? making it Partial later when there is no need for it shouldn't be too much effort, but we're still WIP on the CSOT use case that does need it.

nbbeeken · 2024-10-10T19:03:38Z

test/integration/crud/client_bulk_write.test.ts

+        });
+      });
+
+      it('timeoutMS is used as the timeout for the bulk write', metadata, async function () {


Should we also have tests that separately cover client.bulkWrite throws the expected timeout error? Are there CSOT spec tests for client.bulkWrite? that would suffice

Any other error thrown within the expected deadline isn't capturing the expected behavior, how we communicate the timeout matters if one is to write code against that behavior.

test/integration/client-side-operations-timeout/client_side_operations_timeout.prose.test.ts

test/integration/crud/client_bulk_write.test.ts

Co-authored-by: Warren James <[email protected]>

baileympearson changed the base branch from main to NODE-6090 October 1, 2024 19:21

baileympearson force-pushed the NODE-6090 branch from 17a2fde to 7a12914 Compare October 1, 2024 20:08

baileympearson force-pushed the csot-client-bulk-writ branch from 3cec15c to 6ee5c9c Compare October 1, 2024 20:09

baileympearson changed the title ~~feat(NODE-6090): Implement CSOT logic for connection checkout and ser…~~ feat(NODE-6090): Add CSOT support to client bulk write Oct 1, 2024

baileympearson force-pushed the csot-client-bulk-writ branch 4 times, most recently from d6b99ed to 048868c Compare October 7, 2024 19:36

baileympearson commented Oct 8, 2024

View reviewed changes

baileympearson mentioned this pull request Oct 8, 2024

Specify that CSOT prose test 11 only runs on standalone servers mongodb/specifications#1672

Merged

baileympearson marked this pull request as ready for review October 8, 2024 20:01

baileympearson commented Oct 8, 2024

View reviewed changes

baileympearson changed the title ~~feat(NODE-6090): Add CSOT support to client bulk write~~ feat(NODE-6403): Add CSOT support to client bulk write Oct 9, 2024

baileympearson force-pushed the csot-client-bulk-writ branch 2 times, most recently from b5bf67e to 79aa064 Compare October 9, 2024 14:53

nbbeeken self-assigned this Oct 10, 2024

nbbeeken added the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Oct 10, 2024

nbbeeken requested changes Oct 10, 2024

View reviewed changes

baileympearson requested a review from nbbeeken October 10, 2024 15:50

nbbeeken requested changes Oct 10, 2024

View reviewed changes

baileympearson requested a review from nbbeeken October 10, 2024 17:22

baileympearson force-pushed the NODE-6090 branch from d26a588 to 3591368 Compare October 10, 2024 17:54

baileympearson force-pushed the csot-client-bulk-writ branch from 5792dfd to a1415ce Compare October 10, 2024 17:58

nbbeeken requested changes Oct 10, 2024

View reviewed changes

baileympearson requested a review from nbbeeken October 10, 2024 18:57

nbbeeken requested changes Oct 10, 2024

View reviewed changes

baileympearson force-pushed the NODE-6090 branch from 3591368 to a645d9f Compare October 10, 2024 20:40

nbbeeken removed the Primary Review In Review with primary reviewer, not yet ready for team's eyes label Oct 11, 2024

nbbeeken added the Team Review Needs review from team label Oct 11, 2024

baileympearson added 3 commits October 11, 2024 08:56

client bulk write + comments

1b0d628

comments

d22db74

make test more leniant

69d99c2

baileympearson force-pushed the csot-client-bulk-writ branch from 931c566 to 69d99c2 Compare October 11, 2024 14:56

baileympearson requested a review from nbbeeken October 11, 2024 14:56

nbbeeken approved these changes Oct 11, 2024

View reviewed changes

baileympearson added 2 commits October 11, 2024 09:16

lint

473b8d6

bump threshold again

5f34f43

nbbeeken requested review from durran and W-A-James October 11, 2024 17:23

W-A-James requested changes Oct 11, 2024

View reviewed changes

test/integration/crud/client_bulk_write.test.ts Outdated Show resolved Hide resolved

Update test/integration/crud/client_bulk_write.test.ts

b92ddc1

Co-authored-by: Warren James <[email protected]>

W-A-James approved these changes Oct 14, 2024

View reviewed changes

nbbeeken merged commit 7df1a70 into NODE-6090 Oct 14, 2024
20 of 27 checks passed

nbbeeken deleted the csot-client-bulk-writ branch October 14, 2024 15:38

nbbeeken pushed a commit that referenced this pull request Oct 14, 2024

feat(NODE-6403): add CSOT support to client bulk write (#4261)

450b163

Co-authored-by: Warren James <[email protected]>

baileympearson added a commit that referenced this pull request Oct 21, 2024

feat(NODE-6403): add CSOT support to client bulk write (#4261)

313eaa0

Co-authored-by: Warren James <[email protected]>

baileympearson added a commit that referenced this pull request Oct 21, 2024

feat(NODE-6403): add CSOT support to client bulk write (#4261)

31a1a66

Co-authored-by: Warren James <[email protected]>

nbbeeken pushed a commit that referenced this pull request Nov 1, 2024

feat(NODE-6403): add CSOT support to client bulk write (#4261)

51d55c0

Co-authored-by: Warren James <[email protected]>

nbbeeken pushed a commit that referenced this pull request Nov 5, 2024

feat(NODE-6403): add CSOT support to client bulk write (#4261)

bea3cf3

Co-authored-by: Warren James <[email protected]>

dariakp pushed a commit that referenced this pull request Nov 6, 2024

feat(NODE-6403): add CSOT support to client bulk write (#4261)

365d63b

Co-authored-by: Warren James <[email protected]>

github-actions bot mentioned this pull request Nov 6, 2024

chore(main): release 6.11.0 [skip-ci] #4317

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(NODE-6403): Add CSOT support to client bulk write #4261

feat(NODE-6403): Add CSOT support to client bulk write #4261

baileympearson commented Oct 1, 2024 •

edited by nbbeeken

Loading

baileympearson Oct 8, 2024

baileympearson Oct 8, 2024

baileympearson Oct 8, 2024

nbbeeken Oct 10, 2024

baileympearson Oct 8, 2024

nbbeeken Oct 10, 2024

nbbeeken Oct 10, 2024

nbbeeken Oct 10, 2024

baileympearson Oct 10, 2024

nbbeeken Oct 10, 2024

baileympearson Oct 10, 2024

nbbeeken Oct 10, 2024

nbbeeken Oct 10, 2024

nbbeeken Oct 10, 2024

baileympearson Oct 10, 2024

nbbeeken Oct 10, 2024

baileympearson Oct 10, 2024

nbbeeken Oct 10, 2024

baileympearson Oct 10, 2024

nbbeeken Oct 10, 2024

nbbeeken Oct 10, 2024

nbbeeken Oct 10, 2024

nbbeeken Oct 10, 2024

nbbeeken Oct 10, 2024

nbbeeken Oct 10, 2024

feat(NODE-6403): Add CSOT support to client bulk write #4261

feat(NODE-6403): Add CSOT support to client bulk write #4261

Conversation

baileympearson commented Oct 1, 2024 • edited by nbbeeken Loading

Description

What is changing?

Is there new documentation needed for these changes?

What is the motivation for this change?

Double check the following

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baileympearson commented Oct 1, 2024 •

edited by nbbeeken

Loading